Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EAC-4613 Add observe as alternative to then #19

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

manutzin-te
Copy link
Contributor

Different from then(), observe() returns void and will call the continuation on the executor thread. Any exceptions thrown by the continuation will therefore affect the executor's dispatch thread.

It's intended use case is fire-and-forget futures that we still want to .get() to not silently swallow any exceptions.

Copy link
Contributor

@ggeorgalis ggeorgalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Mostly typos :)

std::forward<TFunc>(cont));
}

//! \brief Observes the input futures and calls the given contionuation function once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: contionuation

std::forward<TFunc>(cont)));
}

//! \brief Observes the input futures and calls the given contionuation function once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: contionuation

namespace thousandeyes {
namespace futures {

//! \brief Observes the input futures and calls the given contionuation function once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: contionuation

std::forward<TFunc>(cont));
}

//! \brief Observes the input futures and calls the given contionuation function once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Comment on lines 237 to 240
EXPECT_DEATH(
{
observe(getExceptionAsync<SomeKindOfError>(), [&](future<void> f) {
f.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think that by EXPECT_DEATH you're testing more things that you have to. Isn't it enough to test EXPECT_THROW(f.get(), ExceptionType)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to capture the fact that the exception is propagated into the dispatcher thread. If we're fine with testing only the fact that observe calls the continuation, then indeed EXPECT_THROW is enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to leave as is. Only, it looks more like an integration test than a unit test.

Copy link
Contributor

@jstinson-te jstinson-te left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - some minor suggestions.

#pragma once

#include <future>
#include <memory>
Copy link
Contributor

@jstinson-te jstinson-te Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: memory is unused but chrono is used, I'd swap this for a chrono include

Comment on lines 242 to 243
// Because f.get() throws, the remaining code in this block
// is never executed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect to never get to this code, why not add a FAIL() << "Should not be executed"; line to incidate the issue to the person running the test?

mutex mtx;
condition_variable cv;

// This will never become true, as the continuation will rethrow the exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a EXPECT_FALSE(success); to the end of the test to make this expectation checked?

Different from then(), observe() returns void and will
call the continuation on the executor thread. Any
exceptions thrown by the continuation will therefore
affect the executor's dispatch thread.

It's intended use case is fire-and-forget futures that
we still want to .get() to not silently swallow any
exceptions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants